-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add cli commons to factor out common parsing code #7301
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit tests and a question but otherwise LGTM
@@ -0,0 +1,7 @@ | |||
plugins { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it overkill to add a README for this module describing it's purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol. i guess if you have to ask it's not overkill :D I'll add it.
void testGetTaggedImageName() { | ||
final String repository = "airbyte/repo"; | ||
final String tag = "12.3"; | ||
assertEquals("airbyte/repo:12.3", Clis.getTaggedImageName(repository, tag)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this method defined? am I being thick? it's not in Clis.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not thick. i fix.
import org.apache.commons.cli.Options; | ||
import org.apache.commons.cli.ParseException; | ||
|
||
public class Clis { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably have at least one test for each public method in this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a small comment on the use of an enum.
} | ||
default -> throw new IllegalStateException("Unexpected value: " + command); | ||
} | ||
|
||
final CommandLine parsed = runParse(options, args, command); | ||
final CommandLine parsed = Clis.parse(args, options, command.toString().toLowerCase()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the command enum is not used here and we use string instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the trade off of DRYing this is that the common parse
function can't really reasonably know about the enum, right?
What